[SPARK-49121][SQL] Support `from_protobuf` and `to_protobuf` for SQL functions by itholic · Pull Request #47716 · apache/spark

itholic · 2024-08-12T08:19:03Z

What changes were proposed in this pull request?

This PR proposes to support from_protobuf and to_protobuf for SQL functions

Similar to #46977

Why are the changes needed?

For improving feature parity with DataFrame API

Does this PR introduce any user-facing change?

This enables from_protobuf and to_protobuf from SQL functions

How was this patch tested?

Added UTs.

Was this patch authored or co-authored using generative AI tooling?

No.

itholic · 2024-08-20T00:18:40Z

cc @HyukjinKwon @hvanhovell I think now it is ready for review. Please take a look when you find some time :-)

Also cc @cloud-fan FYI

xinrong-meng · 2024-08-21T01:05:53Z

nit:
"The .. argument of .. SQL function must be a (constant string).."

Those 6 error messages might be able to reuse a template string and/or a centralized mapping for clarity. Just a thought for minor improvement no need to be in this pr

Thanks! Let me address it from follow-up PR since we have more similar cases from from_avro and to_avro, and I plan to remove those duplications from follow-ups

HyukjinKwon · 2024-08-21T04:12:36Z

Let's also fix the test failure:

[info] - SPARK-49121: from_protobuf and to_protobuf SQL functions *** FAILED *** (622 milliseconds)
[info]   152 did not equal 153 Invalid stopIndex of a query context. Actual:SQLQueryContext(Some(2),Some(7),Some(8),Some(152),Some(
[info]   SELECT from_protobuf(protobuf_data, 42, '/home/runner/work/spark/spark/connector/protobuf/target/generated-test-sources/descriptor-set-sbt.desc', map())
[info]   FROM (
[info]   SELECT
[info]     to_protobuf(
[info]       complex_struct, 'SimpleMessageJavaTypes', '/home/runner/work/spark/spark/connector/protobuf/target/generated-test-sources/descriptor-set-sbt.desc', map()
[info]     ) AS protobuf_data
[info]   FROM protobuf_test_table
[info]   )
[info]   ),None,None) (SparkFunSuite.scala:376)
[info]   org.scalatest.exceptions.TestFailedException:
[info]   at org.scalatest.Assertions.newAssertionFailedException(Assertions.scala:472)
[info]   at org.scalatest.Assertions.newAssertionFailedException$(Assertions.scala:471)
[info]   at org.scalatest.Assertions$.newAssertionFailedException(Assertions.scala:1231)
[info]   at org.scalatest.Assertions$AssertionsHelper.macroAssert(Assertions.scala:1295)
[info]   at org.apache.spark.SparkFunSuite.$anonfun$checkError$4(SparkFunSuite.scala:376)
[info]   at scala.collection.ArrayOps$.foreach$extension(ArrayOps.scala:1323)
[info]   at org.apache.spark.SparkFunSuite.checkError(SparkFunSuite.scala:366)
[info]   at org.apache.spark.sql.protobuf.ProtobufFunctionsSuite.$anonfun$new$151(ProtobufFunctionsSuite.scala:2159)
[info]   at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.scala:18)
[info]   at org.apache.spark.util.SparkErrorUtils.tryWithSafeFinally(SparkErrorUtils.scala:64)
[info]   at org.apache.spark.util.SparkErrorUtils.tryWithSafeFinally$(SparkErrorUtils.scala:61)
[info]   at org.apache.spark.util.Utils$.tryWithSafeFinally(Utils.scala:99)
[info]   at org.apache.spark.sql.test.SQLTestUtilsBase.withTable(SQLTestUtils.scala:308)
[info]   at org.apache.spark.sql.test.SQLTestUtilsBase.withTable$(SQLTestUtils.scala:306)
[info]   at org.apache.spark.sql.protobuf.ProtobufFunctionsSuite.withTable(ProtobufFunctionsSuite.scala:37)
[info]   at org.apache.spark.sql.protobuf.ProtobufFunctionsSuite.$anonfun$new$150(ProtobufFunctionsSuite.scala:2046)
[info]   at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.scala:18)

HyukjinKwon · 2024-08-21T11:04:22Z

[info] - SPARK-49121: from_protobuf and to_protobuf SQL functions *** FAILED *** (607 milliseconds)
[info]   173 did not equal 174 Invalid stopIndex of a query context. Actual:SQLQueryContext(Some(3),Some(2),Some(10),Some(173),Some(
[info]   SELECT
[info]     from_protobuf(protobuf_data, 'SimpleMessageJavaTypes', '/home/runner/work/spark/spark/connector/protobuf/target/generated-test-sources/descriptor-set-sbt.desc', 42)
[info]   FROM (
[info]   SELECT
[info]     to_protobuf(
[info]       complex_struct, 'SimpleMessageJavaTypes', '/home/runner/work/spark/spark/connector/protobuf/target/generated-test-sources/descriptor-set-sbt.desc', map()
[info]     ) AS protobuf_data
[info]   FROM protobuf_test_table
[info]   )
[info]   ),None,None) (SparkFunSuite.scala:376)
[info]   org.scalatest.exceptions.TestFailedException:
[info]   at org.scalatest.Assertions.newAssertionFailedException(Assertions.scala:472)
[info]   at org.scalatest.Assertions.newAssertionFailedException$(Assertions.scala:471)
[info]   at org.scalatest.Assertions$.newAssertionFailedException(Assertions.scala:1231)
[info]   at org.scalatest.Assertions$AssertionsHelper.macroAssert(Assertions.scala:1295)
[info]   at org.apache.spark.SparkFunSuite.$anonfun$checkError$4(SparkFunSuite.scala:376)
[info]   at scala.collection.ArrayOps$.foreach$extension(ArrayOps.scala:1323)
[info]   at org.apache.spark.SparkFunSuite.checkError(SparkFunSuite.scala:366)
[info]   at org.apache.spark.sql.protobuf.ProtobufFunctionsSuite.$anonfun$new$151(ProtobufFunctionsSuite.scala:2196)
[info]   at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.scala:18)
[info]   at org.apache.spark.util.SparkErrorUtils.tryWithSafeFinally(SparkErrorUtils.scala:64)
[info]   at org.apache.spark.util.SparkErrorUtils.tryWithSafeFinally$(SparkErrorUtils.scala:61)
[info]   at org.apache.spark.util.Utils$.tryWithSafeFinally(Utils.scala:99)
[info]   at org.apache.spark.sql.test.SQLTestUtilsBase.withTable(SQLTestUtils.scala:308)
[info]   at org.apache.spark.sql.test.SQLTestUtilsBase.withTable$(SQLTestUtils.scala:306)

…_protobuf

HyukjinKwon · 2024-08-21T23:55:42Z

[info] - SPARK-49121: from_protobuf and to_protobuf SQL functions *** FAILED *** (616 milliseconds)
[info]   "...r-set-sbt.desc', 42)[]" did not equal "...r-set-sbt.desc', 42)[)]" Invalid fragment of a query context. Actual:SQLQueryContext(Some(3),Some(2),Some(10),Some(173),Some(
[info]   SELECT
[info]     from_protobuf(protobuf_data, 'SimpleMessageJavaTypes', '/home/runner/work/spark/spark/connector/protobuf/target/generated-test-sources/descriptor-set-sbt.desc', 42)
[info]   FROM (
[info]   SELECT
[info]     to_protobuf(
[info]       complex_struct, 'SimpleMessageJavaTypes', '/home/runner/work/spark/spark/connector/protobuf/target/generated-test-sources/descriptor-set-sbt.desc', map()
[info]     ) AS protobuf_data
[info]   FROM protobuf_test_table
[info]   )
[info]   ),None,None) (SparkFunSuite.scala:378)
[info]   Analysis:
[info]   "...r-set-sbt.desc', 42)[]" -> "...r-set-sbt.desc', 42)[)]"
[info]   org.scalatest.exceptions.TestFailedException:
[info]   at org.scalatest.Assertions.newAssertionFailedException(Assertions.scala:472)
[info]   at org.scalatest.Assertions.newAssertionFailedException$(Assertions.scala:471)
[info]   at org.scalatest.Assertions$.newAssertionFailedException(Assertions.scala:1231)
[info]   at org.scalatest.Assertions$AssertionsHelper.macroAssert(Assertions.scala:1295)
[info]   at org.apache.spark.SparkFunSuite.$anonfun$checkError$4(SparkFunSuite.scala:378)
[info]   at scala.collection.ArrayOps$.foreach$extension(ArrayOps.scala:1323)
[info]   at org.apache.spark.SparkFunSuite.checkError(SparkFunSuite.scala:366)
[info]   at org.apache.spark.sql.protobuf.ProtobufFunctionsSuite.$anonfun$new$151(ProtobufFunctionsSuite.scala:2196)
[info]   at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.scala:18)
[info]   at org.apache.spark.util.SparkErrorUtils.tryWithSafeFinally(SparkErrorUtils.scala:64)
[info]   at org.apache.spark.util.SparkErrorUtils.tryWithSafeFinally$(SparkErrorUtils.scala:61)

…_protobuf

HyukjinKwon · 2024-08-22T03:49:57Z

Merged to master.

LuciferYang · 2024-08-23T07:34:32Z

    }
  }

+  test("SPARK-49121: from_protobuf and to_protobuf SQL functions") {


@itholic It seems that this test cannot be successfully tested in the Maven daily test, both Java 17 and Java 21：

Java 21: https://github.com/apache/spark/actions/runs/10509632076/job/29116303271

Java 17: https://github.com/apache/spark/actions/runs/10508648823/job/29113000899

also cc @HyukjinKwon

@itholic mind taking a look please?

@itholic @HyukjinKwon Attempting to fix: #47855

Thanks for following up.

github-actions Bot added the SQL label Aug 12, 2024

HyukjinKwon reviewed Aug 16, 2024

View reviewed changes

Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala Outdated

github-actions Bot added the PROTOBUF label Aug 19, 2024

itholic force-pushed the from_to_protobuf branch from 0ddcad7 to c420fe5 Compare August 19, 2024 11:30

itholic marked this pull request as ready for review August 20, 2024 00:15

itholic changed the title ~~[WIP][SPARK-49121][SQL] Support from_protobuf and to_protobuf for SQL functions~~ [SPARK-49121][SQL] Support from_protobuf and to_protobuf for SQL functions Aug 20, 2024

xinrong-meng reviewed Aug 21, 2024

View reviewed changes

[SPARK-49121] Support from_protobuf and to_protobuf for SQL functions

9822da3

itholic force-pushed the from_to_protobuf branch from c78fddf to 9822da3 Compare August 21, 2024 09:43

itholic added 2 commits August 21, 2024 23:39

fix

b0297d0

Merge branch 'master' of https://github.com/apache/spark into from_to…

614cfbf

…_protobuf

itholic added 2 commits August 22, 2024 08:57

fix test

d0dc653

Merge branch 'master' of https://github.com/apache/spark into from_to…

6c47b1d

…_protobuf

HyukjinKwon closed this in b796ecb Aug 22, 2024

LuciferYang reviewed Aug 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SPARK-49121][SQL] Support `from_protobuf` and `to_protobuf` for SQL functions#47716

[SPARK-49121][SQL] Support `from_protobuf` and `to_protobuf` for SQL functions#47716
itholic wants to merge 5 commits into
apache:masterfrom
itholic:from_to_protobuf

itholic commented Aug 12, 2024 •

edited

Loading

Uh oh!

Uh oh!

itholic commented Aug 20, 2024

Uh oh!

xinrong-meng Aug 21, 2024

Uh oh!

itholic Aug 21, 2024

Uh oh!

HyukjinKwon commented Aug 21, 2024

Uh oh!

HyukjinKwon commented Aug 21, 2024

Uh oh!

HyukjinKwon commented Aug 21, 2024

Uh oh!

HyukjinKwon commented Aug 22, 2024

Uh oh!

LuciferYang Aug 23, 2024

Uh oh!

HyukjinKwon Aug 23, 2024

Uh oh!

LuciferYang Aug 23, 2024

Uh oh!

HyukjinKwon Aug 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

itholic commented Aug 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

Uh oh!

itholic commented Aug 20, 2024

Uh oh!

xinrong-meng Aug 21, 2024

Choose a reason for hiding this comment

Uh oh!

itholic Aug 21, 2024

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Aug 21, 2024

Uh oh!

HyukjinKwon commented Aug 21, 2024

Uh oh!

HyukjinKwon commented Aug 21, 2024

Uh oh!

HyukjinKwon commented Aug 22, 2024

Uh oh!

LuciferYang Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

LuciferYang Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Aug 24, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

itholic commented Aug 12, 2024 •

edited

Loading